Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Add protect & unprotect workunit to WU context menu #367

Merged
merged 1 commit into from
Feb 19, 2024

Conversation

dehilsterlexis
Copy link
Contributor

This will require the adding of two functions to the Workunit class in the comms code in the Visualization repository: https://github.com/hpcc-systems/Visualization/blob/trunk/packages/comms/src/ecl/workunit.ts

@dehilsterlexis
Copy link
Contributor Author

@GordonSmith for review

Copy link
Member

@GordonSmith GordonSmith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you opened a PR or Ticket for the Viz repo?

Can you remove the submenu and only show "Protect" when WU is not protected and show "Unprotect" when the WU is protected?

@dehilsterlexis
Copy link
Contributor Author

I did create a ticket (not an issue) for this.

@dehilsterlexis
Copy link
Contributor Author

@GordonSmith will make the changes you requested.

@dehilsterlexis
Copy link
Contributor Author

@GordonSmith come to think of it, I don't believe you can dynamically change the menu. You can only gray things out. So I suggest we gray the appropriate one out.

@GordonSmith
Copy link
Member

GordonSmith commented Dec 15, 2023

It is a little bit tricky - but if you search the code base for:

  • hpccPlatform.isAllWorkunits
  • hpccPlatform.isMyWorkunits

You can see how I worked around the limitation.

@dehilsterlexis
Copy link
Contributor Author

@GordonSmith the setContext command is a global command and works in the case of isAllWorkunits and isMyWorkunits because they are globals that pertain to all tree nodes. The protect only applies to a single node so it can't be used in the same way. It will have to be grayed out using viewItem because viewItem is specific to each tree node.

@GordonSmith
Copy link
Member

Sounds plausible.

@dehilsterlexis dehilsterlexis force-pushed the ADD_PROTECT branch 3 times, most recently from 04d60d5 to d56fad6 Compare January 4, 2024 22:13
@GordonSmith
Copy link
Member

@dehilsterlexis you need to

  1. Bump the version of @hpcc-js/comms in the package.json
  2. Run npm install
  3. Commit package.json + package-lock.json

@dehilsterlexis
Copy link
Contributor Author

@GordonSmith for review

@GordonSmith GordonSmith self-requested a review January 9, 2024 14:27
@@ -93,6 +93,16 @@ export class ECLWatchTree extends Tree {
wuNode.delete();
});

vscode.commands.registerCommand("hpccPlatform.protectWU", (wuNode: ECLWUNode) => {
wuNode.protect();
this.refresh();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure this refresh is needed? If it is, it should wait for the "protect" to complete?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you need it or the menu doesn't update.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this.refresh here.


vscode.commands.registerCommand("hpccPlatform.unprotectWU", (wuNode: ECLWUNode) => {
wuNode.unprotect();
this.refresh();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure this refresh is needed? If it is, it should wait for the "unprotect" to complete?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you need it or the menu doesn't update.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this.refresh here.

}

unprotect() {
this._wu.unprotect();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should refresh the tree?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, that would call it twice.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change to:

this._wu.unprotect().then(() => this._tree.refresh());

@@ -421,7 +439,8 @@ export class ECLWUNode extends Item<ECLWatchTree> {
}

contextValue(): string {
return this._wu.isComplete() ? "ECLWUNodeComplete" : "ECLWUNode";
const prot = this._wu.Protected ? "Protected" : "Unprotected";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change to ECLWUNodeProtected and ECLWUNode respectivly.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bump

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reasons for this change:

  1. Consistency
  2. Avoid a context clash with other extensions (not very likely, but...)

Copy link
Member

@GordonSmith GordonSmith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also update ECLWUNode.getLabel to

    getLabel(): string {
        let primary = this._wu.Wuid;
        const extras: string[] = [];
        if (!this._wu.isComplete() || this._wu.isDeleted()) extras.push(this._wu.State);
        if (!this._tree._myWorkunits && this._wu.Owner) extras.push(this._wu.Owner);
        if (this._wu.Jobname) {
            primary = this._wu.Jobname;
            extras.push(this._wu.Wuid);
        }
        if (this._wu.Protected) {
            extras.push("Protected");
        }
        return extras.length ? `${primary} (${extras.join(", ")})` : primary;
    }

And / or update the icon to show the lock symbol?

@@ -93,6 +93,16 @@ export class ECLWatchTree extends Tree {
wuNode.delete();
});

vscode.commands.registerCommand("hpccPlatform.protectWU", (wuNode: ECLWUNode) => {
wuNode.protect();
this.refresh();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this.refresh here.


vscode.commands.registerCommand("hpccPlatform.unprotectWU", (wuNode: ECLWUNode) => {
wuNode.unprotect();
this.refresh();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this.refresh here.

}

unprotect() {
this._wu.unprotect();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change to:

this._wu.unprotect().then(() => this._tree.refresh());

@@ -421,7 +439,8 @@ export class ECLWUNode extends Item<ECLWatchTree> {
}

contextValue(): string {
return this._wu.isComplete() ? "ECLWUNodeComplete" : "ECLWUNode";
const prot = this._wu.Protected ? "Protected" : "Unprotected";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bump

@@ -421,7 +439,8 @@ export class ECLWUNode extends Item<ECLWatchTree> {
}

contextValue(): string {
return this._wu.isComplete() ? "ECLWUNodeComplete" : "ECLWUNode";
const prot = this._wu.Protected ? "Protected" : "Unprotected";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reasons for this change:

  1. Consistency
  2. Avoid a context clash with other extensions (not very likely, but...)

@dehilsterlexis
Copy link
Contributor Author

dehilsterlexis commented Jan 23, 2024

@GordonSmith for review. The caret in the when clause does not seem to work in this particular when clauses so ECLWUNodeUnprotected was used.

Copy link
Member

@GordonSmith GordonSmith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See the comment WRT to getLabel.

@dehilsterlexis
Copy link
Contributor Author

@GordonSmith for review

@dehilsterlexis
Copy link
Contributor Author

@GordonSmith this is most likely failing because of a version problem. Any suggestions?

@GordonSmith
Copy link
Member

You need to:

npm install

And then commit both the package-lock.json and the package.json.

Did you test its working?

@dehilsterlexis dehilsterlexis force-pushed the ADD_PROTECT branch 2 times, most recently from 3adb820 to 6abd006 Compare January 24, 2024 18:06
@dehilsterlexis dehilsterlexis force-pushed the ADD_PROTECT branch 4 times, most recently from 39a2571 to 938114e Compare January 24, 2024 18:23
@GordonSmith
Copy link
Member

PR tests failing...

@dehilsterlexis
Copy link
Contributor Author

dehilsterlexis commented Jan 25, 2024

@GordonSmith yes I know. I can't seem to get rid of some files in the PR that should not be there. Usually this is fixed by rebasing.

@dehilsterlexis dehilsterlexis force-pushed the ADD_PROTECT branch 3 times, most recently from 5cea82a to 0b98c93 Compare January 25, 2024 14:52
@dehilsterlexis
Copy link
Contributor Author

@GordonSmith for review

package.json Outdated
"command": "hpccPlatform.protectWU",
"category": "ECL",
"title": "%Protect Workunit%",
"enablement": "viewItem =~ /ECLWUNodeComplete/"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think its ok to protect a WU while its running?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think its ok to protect a WU while its running?

package.json Outdated
"command": "hpccPlatform.unprotectWU",
"category": "ECL",
"title": "%Unprotect Workunit%",
"enablement": "viewItem =~ /ECLWUNodeComplete/"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think its ok to unprotect a WU while its running?


getDescription(): string {
return this._result.Value ?? "";
return `${this._result.Name}: ${this._result.Value}`;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer the label + description approach.

@dehilsterlexis dehilsterlexis force-pushed the ADD_PROTECT branch 3 times, most recently from d44f957 to b30e763 Compare January 26, 2024 16:40
Copy link
Member

@GordonSmith GordonSmith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WU Can be protected / unprotected while running

@GordonSmith
Copy link
Member

@dehilsterlexis some comments and needs a rebase.

@dehilsterlexis dehilsterlexis force-pushed the ADD_PROTECT branch 2 times, most recently from 4cf6813 to 088c789 Compare February 13, 2024 14:41
@dehilsterlexis
Copy link
Contributor Author

@GordonSmith for review

package.json Outdated
"command": "hpccPlatform.protectWU",
"category": "ECL",
"title": "%Protect Workunit%",
"enablement": "viewItem =~ /ECLWUNodeComplete/"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think its ok to protect a WU while its running?

Copy link
Member

@GordonSmith GordonSmith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should be able to protect and unprotect a WU before its completed.

@ddehilster
Copy link

@GordonSmith now functions before completed. For review.

@GordonSmith GordonSmith merged commit d255624 into hpcc-systems:trunk Feb 19, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants